Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop column NumFollowers and NumFollowing #24676

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Drop column NumFollowers and NumFollowing #24676

wants to merge 24 commits into from
+86 −136

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented May 12, 2023 •
edited

In old design, we had no user visibility, so we stored them as const variables in db.
Since we added user visibility, we should check whether user can see someone, so there was a fix in #20220.
But we didn't notice these unnecessary const variables in db, as they are dynamically calculated by user_model.GetUserFollowers and user_model.GetUserFollowing.

Discussed in:
#24345 (comment)
#24345 (comment)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 12, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 2023
lunny
lunny previously approved these changes May 12, 2023
View reviewed changes
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 12, 2023
delvh
delvh reviewed May 12, 2023
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I haven't quite understood:
You only remove in this PR.
Where do you calculate it?
I don't think it's a good idea to extract the calculation into another PR.

@KN4CK3R
Copy link
Member

KN4CK3R commented May 13, 2023

The profile page calculates this value always, so nothing is missing. But the API needs that calculation too.

@yp05327
Copy link
Contributor Author

yp05327 commented May 15, 2023 •
edited

What I haven't quite understood: You only remove in this PR. Where do you calculate it? I don't think it's a good idea to extract the calculation into another PR.

It is calculated by user_model.GetUserFollowers and user_model.GetUserFollowing.
In old design, we had no user permission, so we stored them as const variables in db.
Since we added user permission, we should check whether user can see someone, so there was a fix in #20220.
So we have done the calculation, but didn't notice these unnecessary const variables in db, as they are calculated dynamically.

And not only NumFollowers and NumFollowing, but also NumWatches, NumStars, NumForks... should be calculated dynamically and the stored data in db will be unnecessary I think. Maybe we can check them later.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 15, 2023
@yp05327 yp05327 marked this pull request as draft May 15, 2023 02:53
@delvh delvh added the type/deprecation Previously provided functionality is removed label May 15, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 15, 2023
@yardenshoham
Copy link
Member

Document the deprecation in the PR description

@yp05327
Copy link
Contributor Author

yp05327 commented May 18, 2023

Document the deprecation in the PR description

Updated.

@yp05327
Copy link
Contributor Author

yp05327 commented May 18, 2023

After finish #24676 (comment), I will convert this PR into Open state.

@yp05327 yp05327 marked this pull request as ready for review May 19, 2023 08:51
@yp05327 yp05327 requested review from delvh and lunny May 19, 2023 08:51
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels May 19, 2023
@lunny lunny dismissed their stale review May 22, 2023 03:36

See my comments

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 22, 2023
@yp05327 yp05327 requested a review from lunny May 25, 2023 05:06
lunny
lunny reviewed May 25, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 25, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 25, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
@yp05327 yp05327 force-pushed the remove-numfollowers-numfollowing branch from 980aab9 to 8645c5c Compare May 25, 2023 07:53
lunny
lunny reviewed May 25, 2023
View reviewed changes
services/convert/user.go
// signed shall only be set if requester is logged in. authed shall only be set if user is site admin or user himself
func toUser(ctx context.Context, user *user_model.User, signed, authed bool) *api.User {
// accessMode is only used if doer is nil
func toUser(ctx context.Context, user, doer *user_model.User, accessMode perm.AccessMode) *api.User {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like accessMode is always perm.AccessModeNone, maybe we should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If doer is nil, we use accessMode to check whether provide Email info.

@yp05327
Copy link
Contributor Author

yp05327 commented May 26, 2023

image
It seems that there is a bug in sql query.

lunny
lunny reviewed May 26, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 26, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 29, 2023
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 31, 2023
View reviewed changes
services/convert/user.go
}
result.Following = int(count)
} else if accessMode != perm.AccessModeNone {
signed = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An non-login user has read premission to a public repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem? The logic is same to the old code.

lunny
lunny reviewed Apr 18, 2024
View reviewed changes
services/convert/user.go
authed = doer.ID == user.ID || doer.IsAdmin
}
return toUser(ctx, user, signed, authed)
return toUser(ctx, user, doer, perm.AccessModeNone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's unrelated changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny left review comments

@delvh delvh Awaiting requested review from delvh

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/deprecation Previously provided functionality is removed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yp05327 @KN4CK3R @yardenshoham @lunny @delvh @GiteaBot